Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import copy fastpath #1197

Closed
wants to merge 2 commits into from

Conversation

cgwalters
Copy link
Member

Splitting the copy fastpath bit out from #1193


if (!_ostree_repo_ensure_loose_objdir_at (self->objects_dir_fd, loose_path_buf, cancellable, error))
return FALSE;
/* This is yet another variation of glnx_file_copy_at()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, if it's a bare file, we have to chown, right? (Well, unless we're running as the id of the source object, which is probably an optimization we should teach to glnx_file_copy_at).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, realized that after submitting. Will fix soon.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(To clarify we only need to chown for bare repos)

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably d75316c) made this pull request unmergeable. Please resolve the merge conflicts.

This fixes up the last of the embarassing bits I saw from
the stack trace in:
ostreedev#1184

We had a hardlink fast path, but that doesn't apply across
devices, which occurs in two notable cases:

 - Installer ISO with local repo
 - Tools like pungi that copy the repo to a local snapshot

Obviously there are a lot of subtleties here around things like the
bare-user-only conversions as well as exactly what data we copy. I think to get
better test coverage we may want to add `pull-local --no-hardlink` or so.
@cgwalters
Copy link
Member Author

bot, retest this please

@jlebon
Copy link
Member

jlebon commented Sep 26, 2017

@rh-atomic-bot r+ e9d7273

@rh-atomic-bot
Copy link

⌛ Testing commit e9d7273 with merge 8a7a359...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 8a7a359 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants